cmake: linker: Use the same linker for cmake checks and final build #77666
Conversation
Currently, the linker that is used when performing various cmake checks (check_c_compiler_flag, for example) may be different than the linker that will be used during the actual build. This happens as we currently specify '-fuse-ld' to force the appropriate linker a) after many such checks have already happened and b) in a way which is not automatically propagated to check_c_compiler_flag (and friends). As a result, the toolchain's default linker will generally be used for such checks regardless of which linker was selected in Zephyr. This can lead to a number of surprises when building Zephyr, particularly when building with clang. For example: - If the linker is misconfigured, where the build will fail can vary depending on whether the linker is the toolchain's default. When the configured linker happens to be the toolchain's default, the build (helpfully) fails quickly on the checks for a basic working toochain. When the configured linker isn't the default, the build won't fail until the final link steps. - The build can fail due to issues with a linker other than the one configured by the user in Zephyr. For example, LLVM toolchains without lld will generally fail to build Zephyr (the checks for a basic working toochain will fail) for targets where lld is the default in LLVM even if GNU ld is configured in Zephyr and would otherwise be used in the final build. - Flags which are only added if check_c_compiler_flag (or similar) succeeds may be unexpectedly omitted during the final build if the flag is supported in the configured linker but is unsupported in the toolchain's default linker (as check_c_compiler_flag will test using the default one). Note that this isn't limited to clang--even when we are building with Zephyr's SDK and force ld.bfd, we seem to use the 'ld' variant during the cmake checks (though this generally seems fairly harmless compared to mixing ld/lld or other proprietary linkers). To fix this, ensure the appropriate 'fuse-ld' is set early enough and in such a way that the same linker will be used throughout the entire build. Signed-off-by: Jonathon Penix <[email protected]>
jonathonpenix
force-pushed
the
consistent_linker
branch
from
bb95604 to
ad88230
Compare
| @@ -6,6 +6,10 @@ set(CMAKE_LINKER ${LLVMLLD_LINKER}) | |||
|
|
|||
| set_ifndef(LINKERFLAGPREFIX -Wl) | |||
|
|
|||
| list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=lld) | |||
There was a problem hiding this comment.
Two thoughts here:
- Conceptually, setting
-fuse-ldhere rather thantarget_baremetal.cmakemakes sense to me as I don't think-fuse-ldshould be considered a baremetal-specific flag. I don't think this should cause issues for other toolchains (if they relied on it not being specified for non-baremetal cases for some reason, for example) but I'm not sure if there is something I'm missing. oneAPI, for example, seems to use lld but-fuse-ld=lldshould work as expected as best I can tell from the docs. - I dropped the
if(NOT CONFIG_LLVM_USE_LD)check from the old location. This is an unrelated change I guess, but since I was moving this code it didn't make sense to me to keep it--I don't think we should be inlld/target.cmakeunless a)CONFIG_LLVM_USE_LLDwas selected or b) we're using a non-LLVM toolchain at which point I don't thinkCONFIG_LLVM_USE_LDshould be set, so the check is always true. I can readd theifif desired/if I'm misunderstanding though!
There was a problem hiding this comment.
Conceptually, setting -fuse-ld here rather than target_baremetal.cmake makes sense to me as I don't think -fuse-ld should be considered a baremetal-specific flag.
agree. unfortunately the existing toolchain_ld_base and toolchain_ld_baremetal macros has become a bit messy over time and thus diverted from their initial purpose.
A first approach in cleaning up this without introducing functional changes is done here #77887.
Although that cleanup doesn't target the mis-placement of -fuse-ld=lld, then it might have your interest.
I dropped the if(NOT CONFIG_LLVM_USE_LD) check from the old location. ...
I don't think we should be in lld/target.cmake unless a) CONFIG_LLVM_USE_LLD was selected
👍
There was a problem hiding this comment.
agree. unfortunately the existing toolchain_ld_base and toolchain_ld_baremetal macros has become a bit messy over time and thus diverted from their initial purpose.
A first approach in cleaning up this without introducing functional changes is done here #77887. Although that cleanup doesn't target the mis-placement of
-fuse-ld=lld, then it might have your interest.
Thank you for the pointer (and for the patch itself too)! That makes sense to me--I'll fix this up accordingly soon now that that one has landed!
| if((${CMAKE_LINKER} STREQUAL "${CROSS_COMPILE}ld.bfd") OR | ||
| ${GNULD_LINKER_IS_BFD}) | ||
| # ld.bfd was found so let's explicitly use that for linking, see #32237 | ||
| list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=bfd) |
There was a problem hiding this comment.
I think moving this here is valid but I'm not entirely sure. IIUC, previously -fuse-ld=bfd would only be added when toolchain_ld_link_elf is called for a given target (and it's dependencies). By adding this to TOOLCHAIN_LD_FLAGS instead (which will then be added via zephyr_ld_options when toolchain_ld_base is invoked), I think we'll effectively add -fuse-ld=bfd everywhere we link. As toolchain_ld_link_elf is (IIUC) invoked for each of the link stages (pre0/1, final) it seems like this change should be equivalent--are there use cases where we're both linking other things and truly don't want to use the same linker variant as the "main" .elfs?
I don't see anything concerning in 39f06e0 where this was first added, but I'm not sure if I'm missing/misunderstanding something.
There was a problem hiding this comment.
I think this is a fine location.
zephyrbot
requested review from
57300,
jeremybettis,
nashif,
nordicjm,
stephanosio and
tejlmand
| @@ -6,6 +6,10 @@ set(CMAKE_LINKER ${LLVMLLD_LINKER}) | |||
|
|
|||
| set_ifndef(LINKERFLAGPREFIX -Wl) | |||
|
|
|||
| list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=lld) | |||
There was a problem hiding this comment.
Conceptually, setting -fuse-ld here rather than target_baremetal.cmake makes sense to me as I don't think -fuse-ld should be considered a baremetal-specific flag.
agree. unfortunately the existing toolchain_ld_base and toolchain_ld_baremetal macros has become a bit messy over time and thus diverted from their initial purpose.
A first approach in cleaning up this without introducing functional changes is done here #77887.
Although that cleanup doesn't target the mis-placement of -fuse-ld=lld, then it might have your interest.
I dropped the if(NOT CONFIG_LLVM_USE_LD) check from the old location. ...
I don't think we should be in lld/target.cmake unless a) CONFIG_LLVM_USE_LLD was selected
👍
| if((${CMAKE_LINKER} STREQUAL "${CROSS_COMPILE}ld.bfd") OR | ||
| ${GNULD_LINKER_IS_BFD}) | ||
| # ld.bfd was found so let's explicitly use that for linking, see #32237 | ||
| list(APPEND TOOLCHAIN_LD_FLAGS -fuse-ld=bfd) |
There was a problem hiding this comment.
I think this is a fine location.
|
@jonathonpenix I've taken this cleanup PR, adjusted it a bit to adopt to the changes from #77887 and included the changes in #78320 as 789b9df |
Sounds good, thank you! I'll close this PR for now |
Currently, the linker that is used when performing various cmake checks (check_c_compiler_flag, for example) may be different than the linker that will be used during the actual build. This happens as we currently specify '-fuse-ld' to force the appropriate linker a) after many such checks have already happened and b) in a way which is not automatically propagated to check_c_compiler_flag (and friends). As a result, the toolchain's default linker will generally be used for such checks regardless of which linker was selected in Zephyr.
This can lead to a number of surprises when building Zephyr, particularly when building with clang. For example:
Note that this isn't limited to clang--even when we are building with Zephyr's SDK and force ld.bfd, we seem to use the 'ld' variant during the cmake checks (though this generally seems fairly harmless compared to mixing ld/lld or other proprietary linkers).
To fix this, ensure the appropriate 'fuse-ld' is set early enough and in such a way that the same linker will be used throughout the entire build.